Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add YOLO class. #24

Merged
merged 8 commits into from
Aug 31, 2020
Merged

Add YOLO class. #24

merged 8 commits into from
Aug 31, 2020

Conversation

kartikdutt18
Copy link
Member

@kartikdutt18 kartikdutt18 commented Jul 9, 2020

Hey everyone,
This PR aims to YOLO class to the models repo. The following is the TO-DO list:

  • Add PreProcessor Function (Convert DataLoader annotations to YOLO targets). (I'll add this by 10.07.20)

  • Add Detection stage for the YOLO class (Where loss is calculated, feature maps are converted to understandable format).

  • Add YOLO model architecture.

@kartikdutt18
Copy link
Member Author

The build failure occurs because it doesn't have the newer batchnorm yet.

@zoq
Copy link
Member

zoq commented Jul 14, 2020

I guess with mlpack/mlpack#2474 merged, this should build now? Let's rerun the tests.

@kartikdutt18
Copy link
Member Author

I guess with mlpack/mlpack#2474 merged, this should build now? Let's rerun the tests.

Yes, it builds now.

@saksham189
Copy link
Member

saksham189 commented Aug 13, 2020

Can you take a look at why the style build is failing?

@kartikdutt18
Copy link
Member Author

Sure, I will resolve it with the next commit. Thanks for pointing it out.

@KimSangYeon-DGU
Copy link
Member

Hey @kartikdutt18 Can you rebase this PR?

Copy link
Member

@KimSangYeon-DGU KimSangYeon-DGU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments

models/yolo/yolo.hpp Outdated Show resolved Hide resolved
models/yolo/yolo.hpp Outdated Show resolved Hide resolved
if (weights == "voc")
{
// Download weights here.
LoadModel("./../weights/YOLO/yolo" + YOLOVersion + "_voc.bin");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let us know if the weight file is ready. Do you intend to use the weight converter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I do intend to use the weights. I am done with the implementation of detector too, It makes some extra copies which can be avoided which I'll remove and then I'll leave a comment here when everything is ready.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm done with the detector part as well and I'm moving forward with testing the model. If everything works well, I'll add the YOLO model in converter repo for you to test it and then we can merge this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a simple tests in this PR. I haven't added tests for weights because of mlpack/mlpack#2561. I think we can merge this PR since the model is working and any changes would be internal implementation in mlpack. To see if the model is working correctly you can run the following commands :

git clone https://github.com/kartikdutt18/mlpack-PyTorch-Weight-Translator.git
cd mlpack-PyTorch-Weight-Translator
./run.sh yolov1_tiny

With this you will see the model weights being created and transferred into the YOLO model and being used for prediction. You can see that the iou of predictions made by PyTorch model and YOLO model are same i.e. 1.0. Kindly let me know if you face any issues. If this makes sense, I think we can merge this PR and then later add a PR with tests for weights once mlpack/mlpack#2561 is resolved.
(Similar comment from DarkNet).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this we should also merge #29 and #25 as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kartikdutt18 Rather than creating a weight file, how about save it to the weight directory for making it ready-for-use?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would face the same problem as DarkNet since this also uses BatchNorm Layer which has a serialization issue.

Copy link
Member

@KimSangYeon-DGU KimSangYeon-DGU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No further comments from my side.

Copy link
Member

@KimSangYeon-DGU KimSangYeon-DGU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments about styles

models/yolo/yolo.hpp Outdated Show resolved Hide resolved
models/yolo/yolo.hpp Outdated Show resolved Hide resolved
models/yolo/yolo.hpp Outdated Show resolved Hide resolved
models/yolo/yolo.hpp Outdated Show resolved Hide resolved
models/yolo/yolo.hpp Outdated Show resolved Hide resolved
models/yolo/yolo.hpp Outdated Show resolved Hide resolved
models/yolo/yolo.hpp Outdated Show resolved Hide resolved
models/yolo/yolo_impl.hpp Outdated Show resolved Hide resolved
models/yolo/yolo_impl.hpp Outdated Show resolved Hide resolved
Copy link
Member

@KimSangYeon-DGU KimSangYeon-DGU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No further comments from my side.

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second approval provided automatically after 24 hours. 👍

@KimSangYeon-DGU KimSangYeon-DGU merged commit 49dcafb into mlpack:master Aug 31, 2020
@KimSangYeon-DGU
Copy link
Member

Thanks for another great contribution 👍

@kartikdutt18
Copy link
Member Author

Awesome, Thanks a lot @KimSangYeon-DGU and @saksham189 for all the reviews, suggestions and the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants